fix(material-experimental/mdc-chips): Mirror aria-describedby to matChipInput #24551

ByzantineFailure · 2022-03-08T19:39:20Z

Updates mat-chip-grid to associate any ids set for aria-describedby to the matChipInput instance within the grid, if one exists. See #24542 for information on why this is necessary.

Currently this PR creates a setAriaDescribedBy method on matChipInput and calls it from mat-chip-grid whenever mat-chip-grid's setAriaDescribedBy method is called. The internal logic mirrors that of mat-chip-grid. This may not be the best way to go about this change; let me know if there's a better way to do it!

Fixes #24542

crisbeto · 2022-03-09T08:41:46Z

src/material-experimental/mdc-chips/chip-grid.ts

@@ -379,6 +379,10 @@ export class MatChipGrid
   */
  setDescribedByIds(ids: string[]) {
    this._ariaDescribedby = ids.join(' ');
+


I'm wondering whether we should be setting aria-describedby on the chip grid at all, considering that it never receives focus.

That's a good point.

I don't see any harm in keeping it on the chip grid itself, but I also don't see any harm in removing it either.

The only (extremely minor) concern I have is if the focus management of the grid instance changes to focus the grid element itself at some point. That's not the case here, though, and I don't really think it's a big deal if we remove it.

All of that is to say: I can remove the attribute from the grid if you'd like, just let me know!

I think that we should get rid of it, otherwise it's misleading.

Works for me! We still need to keep the property present on the control to satisfy the MatFormFieldControl interface, but we can at least remove the property binding.

Actually maybe not lemme dig further into that.

It's defined on MatChipSet, which means we'll need to keep the property itself but can remove the binding.

ByzantineFailure · 2022-03-09T19:34:41Z

src/material-experimental/mdc-chips/chip-input.ts

@@ -73,6 +74,9 @@ export class MatChipInput implements MatChipTextControl, AfterContentInit, OnCha
  /** Used to prevent focus moving to chips while user is holding backspace */
  private _focusLastChipOnBackspace: boolean;

+  /** Value for ariaDescribedby property */
+  private _ariaDescribedby?: string;


Currently this attribute is causing the linter to fail since it's not referenced by any of the TS code. Is there a way to suppress the linter for this attribute or is there another way we can handle that?

Since it's used in a host binding, it should be changed to a public property. That'll silence the linter as well.

Makes sense. Changing.

crisbeto · 2022-03-10T13:58:09Z

src/material-experimental/mdc-chips/chip-grid.ts

@@ -379,6 +379,10 @@ export class MatChipGrid
   */
  setDescribedByIds(ids: string[]) {
    this._ariaDescribedby = ids.join(' ');
+


I think that we should get rid of it, otherwise it's misleading.

crisbeto · 2022-03-10T13:58:29Z

src/material-experimental/mdc-chips/chip-grid.ts

@@ -379,6 +379,10 @@ export class MatChipGrid
   */
  setDescribedByIds(ids: string[]) {
    this._ariaDescribedby = ids.join(' ');
+
+    if (this._chipInput) {
+      this._chipInput.setDescribedByIds(ids);


Would this have account for IDs that were assigned before the input was defined?

I don't think we have to deal with that in this particular method -- any time an error spawns this method will be called with all relevant ids and set the complete value of the property bound to the template.

The window for this to happen is very small: We assert that a chipInput is present during ngAfterViewInit(), so the error that we're wanting to propagate would need to occur and be set before that time. With this in mind we can handle this case by calling setDescribedByIds after the assertion.

The downside of doing this is it means we need to keep the value of this._ariaDescribedby up to date in this component as well. Probably a best practice anyway since it's the one implementing MatFormFieldControl.

crisbeto · 2022-03-10T13:59:05Z

src/material-experimental/mdc-chips/chip-input.ts

@@ -73,6 +74,9 @@ export class MatChipInput implements MatChipTextControl, AfterContentInit, OnCha
  /** Used to prevent focus moving to chips while user is holding backspace */
  private _focusLastChipOnBackspace: boolean;

+  /** Value for ariaDescribedby property */
+  private _ariaDescribedby?: string;


Since it's used in a host binding, it should be changed to a public property. That'll silence the linter as well.

crisbeto · 2022-03-15T10:05:35Z

It looks like there's still a lint failure.

ByzantineFailure · 2022-03-16T19:41:13Z

I think the e2e failure here is a flake, but am not necessarily able to determine that conclusively from the test output. Is there a way I can re-run the e2e tests on CI?

crisbeto

One more nit otherwise LGTM.

crisbeto · 2022-03-17T10:17:59Z

src/material-experimental/mdc-chips/chip-grid.ts

@@ -337,6 +335,10 @@ export class MatChipGrid
  /** Associates an HTML input element with this chip grid. */
  registerInput(inputElement: MatChipTextControl): void {
    this._chipInput = inputElement;
+
+    if (this._ariaDescribedby) {
+      this._chipInput.setDescribedByIds(this._ariaDescribedby.split(' '));


Instead of having to split the IDs here, maybe we should just save the string array?

This field comes from MatChipSet, which MatChipGridBase extends:

components/src/material-experimental/mdc-chips/chip-set.ts

Line 142 in f05e65a

_ariaDescribedby: string;

The field is defined as a string. I'm more than okay to save the values in an array, but I suspect we'll want to keep the value of _ariaDescribedby up to date regardless.

If we're comfortable introducing a new field to store the array, I'm more than happy to do so; let me know one way or the other.

Hmm, I think that the aria-describedby on MatChipSet isn't doing much since the element isn't focusable. It should be moved to the chip listbox.

crisbeto

LGTM

andrewseguin · 2022-03-24T16:28:50Z

src/material-experimental/mdc-chips/chip-grid.ts

+    this._ariaDescribedbyIds = ids;
+
+    if (this._chipInput) {
+      this._chipInput.setDescribedByIds(ids);


This gets called while the form field gets initialized after content is initialized:

MatChipInput.setDescribedByIds (chip-input.ts) MatChipGrid.setDescribedByIds (chip-grid.ts) MatFormField._syncDescribedByIds (form-field.ts) MatFormField._initializeSubscript (form-field.ts) MatFormField.ngAfterContentInit (form-field.ts)

This caused a ExpressionChangedAfterCheckedError in an application inside Google. It's probably related to how the grid/input content is provided with an ngIf. @crisbeto Is it valid to wrap this in a setTimeout to avoid change detection issues?

Yeah, let's try it with the timeout and we can re-evaluate if it causes even more failures because the timeouts haven't been flushed.

Timeout didn't cause any issues, we can replace the logic with this to land the change

if (this._chipInput) { // Use a setTimeout in case this is being run during change detection // and the chip input has already determined its host binding for // aria-describedBy. setTimeout(() => { this._chipInput.setDescribedByIds(ids); }, 0); }

Just a heads up that this would be best done ASAP so we don't need to re-run internal tests

Sorry about the delay here, been a busy couple of days.

Still debugging a unit test but will push a commit with this change ASAP.

Sounds good, thanks :)

Okay, should be set to go!

Great, we'll start loading up approvals internally to get this in

…hipInput Updates mat-chip-grid to associate any ids set for aria-describedby to the matChipInput instance within the grid, if one exists. Removes the aria-describedby attribute on the grid itself since it never receives focus. Fixes angular#24542

…hipInput (#24551) Updates mat-chip-grid to associate any ids set for aria-describedby to the matChipInput instance within the grid, if one exists. Removes the aria-describedby attribute on the grid itself since it never receives focus. Fixes #24542 (cherry picked from commit 1bc98ec)

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/cdk](https://github.com/angular/components) | dependencies | patch | [`13.3.1` -> `13.3.2`](https://renovatebot.com/diffs/npm/@angular%2fcdk/13.3.1/13.3.2) | | [@angular/material](https://github.com/angular/components) | dependencies | patch | [`13.3.1` -> `13.3.2`](https://renovatebot.com/diffs/npm/@angular%2fmaterial/13.3.1/13.3.2) | --- ### Release Notes <details> <summary>angular/components</summary> ### [`v13.3.2`](https://github.com/angular/components/blob/HEAD/CHANGELOG.md#1332-flannel-flamingo-2022-03-30) [Compare Source](angular/components@13.3.1...13.3.2) ##### cdk | Commit | Type | Description | | -- | -- | -- | | [48968719fc](angular/components@4896871) | fix | **a11y:** live announcer promise never resolved if new announcement comes in ([#24700](angular/components#24700)) | | [e9734a9c66](angular/components@e9734a9) | fix | **testing:** entering negative number values not working with reactive forms ([#24656](angular/components#24656)) | ##### material | Commit | Type | Description | | -- | -- | -- | | [c677f11ed8](angular/components@c677f11) | fix | **button-toggle:** ripples not clipping correctly in safari ([#12311](angular/components#12311)) | | [20af3e7c9d](angular/components@20af3e7) | fix | **chips:** ripple not clipped on safari ([#21495](angular/components#21495)) | | [d04e7c9b69](angular/components@d04e7c9) | fix | **core:** unable to override tag selectors inside .mat-typography ([#14617](angular/components#14617)) | | [9490a31641](angular/components@9490a31) | fix | **list:** not working correctly when list item is used as a button ([#13617](angular/components#13617)) | | [b07ae4ccc4](angular/components@b07ae4c) | fix | **menu:** clicks on disabled item closing the menu ([#19183](angular/components#19183)) | | [e85777712a](angular/components@e857777) | fix | **radio:** set tabindex based on selected state ([#18081](angular/components#18081)) | | [7f274dc96f](angular/components@7f274dc) | fix | **snack-bar:** ensure that the snack bar always runs inside the NgZone ([#24611](angular/components#24611)) | | [a5aa87502b](angular/components@a5aa875) | fix | **tabs:** focus wrapping back to selected label when using shift + tab ([#14194](angular/components#14194)) | | [04f4937b75](angular/components@04f4937) | fix | **tabs:** update tab state when active tab is swapped out ([#24164](angular/components#24164)) | ##### material-experimental | Commit | Type | Description | | -- | -- | -- | | [a704913d2b](angular/components@a704913) | fix | **mdc-button:** extended fab touch target not covering entire button ([#24322](angular/components#24322)) | | [23e7b8e6c1](angular/components@23e7b8e) | fix | **mdc-chips:** make it easier to customize chip typography ([#24632](angular/components#24632)) | | [518022288b](angular/components@5180222) | fix | **mdc-chips:** Mirror aria-describedby to matChipInput ([#24551](angular/components#24551)) | | [9497b02f8b](angular/components@9497b02) | fix | **mdc-slider:** update layout when container resizes ([#24648](angular/components#24648)) | | [e5c025dff4](angular/components@e5c025d) | fix | **mdc-slider:** use passive event listeners ([#24675](angular/components#24675)) | #### Special Thanks Artur Androsovych, ByzantineFailure, David Gonzalez, Dilyorbek, Kristiyan Kostadinov, Naveen, Paul Gschwendtner, Raí Siqueira, Shivam Sethi, Wagner Maciel and Zach Arend  </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1275 Reviewed-by: 6543 <6543@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

…hipInput (angular#24551) Updates mat-chip-grid to associate any ids set for aria-describedby to the matChipInput instance within the grid, if one exists. Removes the aria-describedby attribute on the grid itself since it never receives focus. Fixes angular#24542

angular-automatic-lock-bot · 2022-04-25T00:04:17Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

ByzantineFailure requested a review from mmalerba as a code owner March 8, 2022 19:39

crisbeto reviewed Mar 9, 2022

View reviewed changes

ByzantineFailure commented Mar 9, 2022

View reviewed changes

crisbeto reviewed Mar 10, 2022

View reviewed changes

ByzantineFailure force-pushed the associate-errors-with-chip-input branch 2 times, most recently from 760ba6d to 8d91c51 Compare March 15, 2022 19:57

crisbeto reviewed Mar 17, 2022

View reviewed changes

ByzantineFailure force-pushed the associate-errors-with-chip-input branch 2 times, most recently from 4ec71a4 to 195cbfa Compare March 21, 2022 19:39

crisbeto approved these changes Mar 21, 2022

View reviewed changes

crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Mar 21, 2022

andrewseguin reviewed Mar 24, 2022

View reviewed changes

ByzantineFailure force-pushed the associate-errors-with-chip-input branch from 195cbfa to 12088c5 Compare March 25, 2022 19:38

ByzantineFailure force-pushed the associate-errors-with-chip-input branch from 12088c5 to e1a1b4f Compare March 25, 2022 19:57

andrewseguin merged commit 1bc98ec into angular:master Mar 25, 2022

sync-by-unito bot mentioned this pull request Mar 31, 2022

build(deps): bump @angular/cdk from 12.2.4 to 13.3.2 numbersprotocol/capture-lite#1463

Closed

sync-by-unito bot mentioned this pull request Apr 22, 2022

build(deps): bump @angular/cdk from 12.2.4 to 13.3.4 numbersprotocol/capture-lite#1541

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(material-experimental/mdc-chips): Mirror aria-describedby to matChipInput #24551

fix(material-experimental/mdc-chips): Mirror aria-describedby to matChipInput #24551

ByzantineFailure commented Mar 8, 2022

crisbeto Mar 9, 2022

ByzantineFailure Mar 9, 2022

crisbeto Mar 10, 2022

ByzantineFailure Mar 10, 2022

ByzantineFailure Mar 10, 2022

ByzantineFailure Mar 10, 2022

ByzantineFailure Mar 9, 2022

crisbeto Mar 10, 2022

ByzantineFailure Mar 10, 2022

crisbeto Mar 10, 2022

crisbeto Mar 10, 2022

ByzantineFailure Mar 15, 2022

crisbeto Mar 10, 2022

crisbeto commented Mar 15, 2022

ByzantineFailure commented Mar 16, 2022

crisbeto left a comment

crisbeto Mar 17, 2022

ByzantineFailure Mar 17, 2022

crisbeto Mar 17, 2022

ByzantineFailure Mar 21, 2022

crisbeto left a comment

andrewseguin Mar 24, 2022

crisbeto Mar 24, 2022

andrewseguin Mar 25, 2022

andrewseguin Mar 25, 2022

ByzantineFailure Mar 25, 2022

andrewseguin Mar 25, 2022

ByzantineFailure Mar 25, 2022

andrewseguin Mar 25, 2022

angular-automatic-lock-bot bot commented Apr 25, 2022

fix(material-experimental/mdc-chips): Mirror aria-describedby to matChipInput #24551

fix(material-experimental/mdc-chips): Mirror aria-describedby to matChipInput #24551

Conversation

ByzantineFailure commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Mar 15, 2022

ByzantineFailure commented Mar 16, 2022

crisbeto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Apr 25, 2022